Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove the ffaker sample data dependency (revised) #2163

Merged
merged 1 commit into from
Aug 22, 2017
Merged

Remove the ffaker sample data dependency (revised) #2163

merged 1 commit into from
Aug 22, 2017

Conversation

swcraig
Copy link
Contributor

@swcraig swcraig commented Aug 17, 2017

Addresses comments brought up in @cbrunsdon's original PR.

Original PR description

Continues the idea of #2082, this removes the dependency of ffaker but
also yanks it out of the sample requirement. I generated a small data
set with 15.times.map { FFaker::Name.first_name } etc.

@swcraig
Remove the ffaker sample data dependency
53af716 
Continues the idea of #2082, this removes the dependency of ffaker but
also yanks it out of the sample requirement. I generated a small data
set with 15.times.map { FFaker::Name.first_name } etc.
@swcraig
Copy link
Contributor Author

swcraig commented Aug 17, 2017

#2140 (comment)

@tvdeyen I didn't change the name arrays to use %w syntax because the other arrays in this file use multi-word values and I felt that consistency in the file would be favoured. Instead I spread the arrays over multiple lines when they grew past 80 characters. Let me know if you want me to revert the arrays to sit on single lines again (or whatever solution you find reasonable).

tvdeyen
tvdeyen approved these changes Aug 17, 2017
View reviewed changes
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'totally fine with the array syntax 👌

@tvdeyen tvdeyen mentioned this pull request Aug 17, 2017
Closed
cbrunsdon
cbrunsdon approved these changes Aug 17, 2017
View reviewed changes
Copy link
Contributor

@cbrunsdon cbrunsdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good by me, thanks for finishing this off sebastian

mamhoff
mamhoff approved these changes Aug 18, 2017
View reviewed changes
Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @swcraig, nice work!

@mtomov
Copy link
Contributor

mtomov commented Aug 18, 2017

Thanks!

@cbrunsdon cbrunsdon merged commit 53ee38a into solidusio:master Aug 22, 2017
This was referenced Sep 7, 2017
Merged
Merged
Closed
This was referenced Sep 14, 2017
Merged
Merged
Merged
Closed
Merged
Closed
@jhawthorn jhawthorn mentioned this pull request Oct 31, 2017
Merged
fastjames pushed a commit to geminimvp/solidus_subscriptions that referenced this pull request Aug 1, 2018
@swcraig @fastjames
Add ffaker dependency to gemspec
9bc5b62 
ffaker was removed as a runtime dependency of Solidus. As of right now,
this removal is only on Solidus master (but it is slated for v2.4).

Even though this extension does not use ffaker directly, Solidus
complains that its factories require the library and so the tests fail
without this.

PR removing ffaker from Solidus:
solidusio/solidus#2163
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mamhoff mamhoff mamhoff approved these changes

@cbrunsdon cbrunsdon cbrunsdon approved these changes

@tvdeyen tvdeyen tvdeyen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@swcraig @mtomov @tvdeyen @cbrunsdon @mamhoff